-
Notifications
You must be signed in to change notification settings - Fork 914
[FEAT] Add support for Enterprise Cost Centers #3000
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
👋 Hi! Thank you for this contribution! Just to let you know, our GitHub SDK team does a round of issue and PR reviews twice a week, every Monday and Friday! We have a process in place for prioritizing and responding to your input. Because you are a part of this community please feel free to comment, add to, or pick up any issues/PRs that are labeled with |
|
We're trying to move towards using Context-aware provider functions, could you update all schemas to use those? So ReadContext etc |
13dcd40 to
1ab2c0f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds comprehensive support for GitHub Enterprise Cost Centers, enabling Terraform management of cost center resources and their assignments. The implementation addresses issue #2739 by leveraging the newly public GitHub Enterprise billing API.
Key changes:
- Introduces
github_enterprise_cost_centerresource for creating, updating, and archiving cost centers with authoritative management of user, organization, and repository assignments - Adds
github_enterprise_cost_centerandgithub_enterprise_cost_centersdata sources for querying cost center information - Implements robust resource assignment synchronization with batching (50 resources per request) and retry logic for transient failures
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| website/github.erb | Adds navigation links for the new data sources and resource |
| website/docs/r/enterprise_cost_center.html.markdown | Documents the cost center resource with examples, arguments, attributes, and import instructions |
| website/docs/d/enterprise_cost_center.html.markdown | Documents the single cost center data source for retrieving by ID |
| website/docs/d/enterprise_cost_centers.html.markdown | Documents the data source for listing cost centers with optional state filtering |
| github/util_cost_centers.go | Provides utility functions for API interactions including CRUD operations and resource assignment management |
| github/resource_github_enterprise_cost_center.go | Implements the cost center resource with full lifecycle management and authoritative assignment synchronization |
| github/provider.go | Registers the new resource and data sources with the provider |
| github/resource_github_enterprise_cost_center_test.go | Provides acceptance tests covering create, update, assignment changes, and import scenarios |
| github/data_source_github_enterprise_cost_centers_test.go | Tests the list data source with state filtering |
| github/data_source_github_enterprise_cost_centers.go | Implements the data source for listing cost centers |
| github/data_source_github_enterprise_cost_center_test.go | Tests the single cost center data source retrieval |
| github/data_source_github_enterprise_cost_center.go | Implements the data source for retrieving a specific cost center by ID |
| examples/cost_centers/main.tf | Provides a comprehensive example demonstrating resource and data source usage |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
I built and tested your branch @vmvarela and it works great! I was able to read, import, and create cost centers and resources using your examples and a few of my own. Thank you for the work. |
a3e128c to
0bdad0f
Compare
3314826 to
1d13386
Compare
deiga
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Half-way through. Please consider if my comments could apply to other places in your changes as well, I wasn't able to comment on every duplicate thing
vmvarela
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the thorough review @deiga! I've addressed all the feedback. Here's a summary of the changes:
Implemented:
- ✅ Added top-level
Descriptionattribute to all resources and data sources - ✅ Removed
ctx = context.WithValue(ctx, ctxId, ...)pattern from all files - ✅ Replaced
log.Printfwithtflog.Infoand addedtflog.Warnfor 404 removals - ✅ Used
diag.Errorfinstead ofdiag.FromErr(fmt.Errorf(...)) - ✅ Create and Update no longer call Read - computed fields are set directly
- ✅ Extracted anonymous retry functions to named functions (
retryCostCenterAddResources,retryCostCenterRemoveResources) - ✅ Changed import ID separator from
/to:and usingparseTwoPartID - ✅ Replaced
stringSliceToAnySlicewith existingflattenStringList - ✅ Used
http.StatusNotFoundand other constants instead of magic numbers - ✅ Moved
is404toutil.go - ✅ Updated tests to use
providerFactoriesandskipUnlessEnterprise(t) - ✅ Added
owner = var.enterprise_slugto example provider config - ✅ Used
buildTwoPartIDin data source - ✅ Inlined check variables in tests
Regarding questions:
- The
resourcesattribute provides the raw API view whileusers,organizations,repositoriesare filtered convenience attributes. I kept both for flexibility and debugging purposes. diffStringSlicesdiffers fromsetChanges- it compares string slices directly whilesetChangesworks with Terraform state changes. They serve different purposes.- In Create, we only fetch from API after assignments are configured to populate computed fields, since we're not calling Read anymore.
All tests pass and the code compiles without errors.
- Replace custom API implementation with go-github v81 Enterprise.* methods - Remove util_cost_centers.go and consolidate helpers into resource file - Fix deprecated GetOkExists usage, replaced with GetOk - Fix GitHub capitalization in documentation page titles
- Remove redundant 'resources' attribute (use typed users/organizations/repositories instead) - Remove unnecessary GET call in Create (use POST response values directly) - Remove http.StatusNotFound from retryable errors - Add Description to all nested attributes in cost_centers data source - Use len() for count checks in tests instead of hardcoded values - Move is404() helper to util.go - Use testAccConfig struct for enterprise test variables
8bf87d2 to
b8c4197
Compare
Resolves #2739
Before the change?
After the change?
NOTE: This API (billing) has no support for APP or fine-grained tokens.
Pull request checklist
Does this introduce a breaking change?
Please see our docs on breaking changes to help!